Skip to content
This repository has been archived by the owner on Aug 16, 2022. It is now read-only.

feat: Generic list and detail #1000

Merged
merged 23 commits into from
Jul 11, 2022
Merged

feat: Generic list and detail #1000

merged 23 commits into from
Jul 11, 2022

Conversation

bbernays
Copy link
Contributor

@bbernays bbernays commented Jun 3, 2022 •
edited
Loading

🎉 Thank you for making CloudQuery awesome by submitting a PR 🎉

Summary

This pr Attempts to centralize the parallelization logic for resources in order to provide a singular location for tuning to occur (if necessary). While this initial implementation still has a hardcoded number of go routines, in the future we could swap out that hard coded number for a dynamic value based on resource utilization.

Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests. Learn more about testing here 🧪
  • Update the docs by running go run ./docs/docs.go and committing the changes 📃
  • If adding a new resource, add relevant Terraform files in a separate PR 📂
  • Ensure the status checks below are successful ✅

@bbernays bbernays requested a review from a team as a code owner June 3, 2022 14:34
@bbernays bbernays requested review from yevgenypats and removed request for a team June 3, 2022 14:34
@bbernays bbernays changed the title Generic list and detail feat: Generic list and detail Jun 3, 2022
@bbernays bbernays linked an issue Jun 3, 2022 that may be closed by this pull request
Create Reference Implementation Paginated List + Detail Fetch #927
Closed
@bbernays bbernays requested review from roneli and disq June 3, 2022 15:04
roneli
roneli reviewed Jun 6, 2022
View reviewed changes
Copy link
Contributor

@roneli roneli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, had a few comments.

client/helpers.go Outdated Show resolved Hide resolved
client/helpers.go Outdated Show resolved Hide resolved
@bbernays bbernays requested a review from roneli June 8, 2022 15:51
disq
disq reviewed Jun 8, 2022
View reviewed changes
client/helpers.go Outdated Show resolved Hide resolved
yevgenypats
yevgenypats previously requested changes Jun 9, 2022
View reviewed changes
Copy link
Member

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless this is a blocker for a client I would hold off any non critical speed and new features for aws provider until we are able to stabilize it and get around bugs/sentry.

disq
disq reviewed Jun 9, 2022
View reviewed changes
client/helpers.go Outdated Show resolved Hide resolved
@disq
Copy link
Member

disq commented Jun 9, 2022

This should ideally be refactored in the SDK maybe? not sure

@bbernays
Copy link
Contributor Author

bbernays commented Jun 9, 2022

Unless this is a blocker for a client I would hold off any non critical speed and new features for aws provider until we are able to stabilize it and get around bugs.

I agree that we should not be adding more, but this (IMO) is a stabilizing change... We currently have at least 3 different implementations of this pattern that we are trying to standardize on a single implementation.

@bbernays
Copy link
Contributor Author

bbernays commented Jun 9, 2022

This should ideally be refactored in the SDK maybe? not sure

I think you are correct, but I am not trying to get ahead of ourselves. I am trying to be slow/deliberate going from resolver --> client --> sdk.

I think I would only put this in the SDK if we implemented the same pattern in multiple providers

@yevgenypats
Copy link
Member

@bbernays can you please point me to the 3 places in the code we use this pattern?

@bbernays
Copy link
Contributor Author

bbernays commented Jun 9, 2022

@bbernays can you please point me to the 3 places in the code we use this pattern?

cq-provider-aws/resources/services/athena/data_catalogs.go

Lines 221 to 249 in 53d391b

var sem = semaphore.NewWeighted(int64(MAX_GOROUTINES))
for {
response, err := svc.ListDataCatalogs(ctx, &input, func(options *athena.Options) {
options.Region = c.Region
})
if err != nil {
return diag.WrapError(err)
}
errs, ctx := errgroup.WithContext(ctx)
for _, d := range response.DataCatalogsSummary {
if err := sem.Acquire(ctx, 1); err != nil {
return diag.WrapError(err)
}
func(summary types.DataCatalogSummary) {
errs.Go(func() error {
defer sem.Release(1)
return fetchDataCatalog(ctx, res, svc, c.Region, summary)
})
}(d)
}
err = errs.Wait()
if err != nil {
return diag.WrapError(err)
}
if aws.ToString(response.NextToken) == "" {
break
}
input.NextToken = response.NextToken
}

cq-provider-aws/resources/services/athena/work_groups.go

Lines 416 to 442 in 53d391b

for {
response, err := svc.ListWorkGroups(ctx, &input, func(options *athena.Options) {
options.Region = c.Region
})
if err != nil {
return diag.WrapError(err)
}
errs, ctx := errgroup.WithContext(ctx)
for _, d := range response.WorkGroups {
if err := sem.Acquire(ctx, 1); err != nil {
return diag.WrapError(err)
}
func(summary types.WorkGroupSummary) {
errs.Go(func() error {
defer sem.Release(1)
return fetchWorkGroup(ctx, res, svc, c.Region, summary)
})
}(d)
}
err = errs.Wait()
if err != nil {
return diag.WrapError(err)
}
if aws.ToString(response.NextToken) == "" {
break
}
input.NextToken = response.NextToken

cq-provider-aws/resources/services/sagemaker/training_jobs.go

Lines 590 to 617 in 53d391b

for {
response, err := svc.ListTrainingJobs(ctx, &config, func(options *sagemaker.Options) {
options.Region = c.Region
})
if err != nil {
return diag.WrapError(err)
}
errs, ctx := errgroup.WithContext(ctx)
for _, d := range response.TrainingJobSummaries {
if err := sem.Acquire(ctx, 1); err != nil {
return diag.WrapError(err)
}
func(summary types.TrainingJobSummary) {
errs.Go(func() error {
defer sem.Release(1)
return fetchTrainingJobDefinition(ctx, res, svc, c.Region, summary)
})
}(d)
}
err = errs.Wait()
if err != nil {
return diag.WrapError(err)
}
if aws.ToString(response.NextToken) == "" {
break
}
config.NextToken = response.NextToken
}

cq-provider-aws/resources/services/ecs/task_definitions.go

Lines 647 to 678 in 53d391b

for {
listClustersOutput, err := svc.ListTaskDefinitions(ctx, &config, func(o *ecs.Options) {
o.Region = region
})
if err != nil {
return diag.WrapError(err)
}
if len(listClustersOutput.TaskDefinitionArns) == 0 {
return nil
}
errs, ctx := errgroup.WithContext(ctx)
for _, t := range listClustersOutput.TaskDefinitionArns {
if err := sem.Acquire(ctx, 1); err != nil {
return diag.WrapError(err)
}
func(arn string) {
errs.Go(func() error {
defer sem.Release(1)
return fetchEcsTaskDefinition(ctx, res, svc, region, arn)
})
}(t)
}
err = errs.Wait()
if err != nil {
return diag.WrapError(err)
}
if listClustersOutput.NextToken == nil {
break
}
config.NextToken = listClustersOutput.NextToken
}

@bbernays bbernays mentioned this pull request Jun 20, 2022
Closed
7 tasks
disq
disq reviewed Jun 22, 2022
View reviewed changes
client/helpers.go Outdated Show resolved Hide resolved
@bbernays bbernays requested a review from disq June 24, 2022 12:24
disq
disq approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@disq disq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with couple nits

client/helpers.go Outdated Show resolved Hide resolved
resources/services/athena/work_groups.go Outdated Show resolved Hide resolved
@bbernays bbernays dismissed yevgenypats’s stale review July 11, 2022 13:16

Old review, issues/questions have been addressed

@bbernays bbernays merged commit 16217c8 into cloudquery:main Jul 11, 2022
@bbernays bbernays deleted the generic-list-and-detail branch July 11, 2022 13:49
@cq-bot cq-bot mentioned this pull request Jul 11, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@disq disq disq approved these changes

@roneli roneli Awaiting requested review from roneli

@yevgenypats yevgenypats Awaiting requested review from yevgenypats

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create Reference Implementation Paginated List + Detail Fetch
4 participants
@bbernays @disq @yevgenypats @roneli